-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add declarative smart answer helper methods #4527
Conversation
Awesome 🎉 Thanks @kevindew - it does read rather nicely! I'd be happy with this change - what you've got in draft looks good. Think we just need some docs and tests for the new syntax. Then another PR or two to roll out the changes to the other smart answers? Originally, I was worried about someone using the wrong helper method for content that can on be certain format (e.g. |
97e1145
to
64e1d23
Compare
64e1d23
to
cbd853a
Compare
Thanks @theseanything I've made some updates for this now. I've updated the documentation, added tests for the functionality and I've also enabled the ability for smart answer flow files to have an extension of Regarding your concern of someone using Previously I wasn't too concerned about that kind of mis-use, as it seems just one of many ways someone could make a mistake but it does seem like doing so could create a rather transparent issue. I've also realised that having this in will help with the migration over to this approach since it will break rather indignantly compared to succeeding quietly 😄 I think this is now ready for review. So I'll mark this so and open a draft of the big migration of answers 😬 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kevindew! - I've added a few comments.
cbd853a
to
871169a
Compare
Thanks for the review @theseanything 👍 Something else has just come up so I've left this for now with just comments on your doc comments. Will return later. I've also removed the applying of this to the coronavirus example since it'll need rebasing and I think it's already served it's purpose as a demo. |
@kevindew np!! No rush! - Feel free to hit the re-request review button 🛎️ when your ready! |
95a797b
to
0215636
Compare
@theseanything this is good for another review - thanks 🐱 |
This adds `text_for`, `govspeak_for` and `html_for` methods intended for the smart answer flow methods as a way for users to define content for a smart answer. These are intended to replace the `render_content_for` helper that has logic to determine the content type. This is set-up to be backwards compatible with the render_content_for method so that both can co-exist for a period of time to allow migration. In doing this I also simplified a few things. We don't make any use of options when calling `content_for` so these methods don't need to support an option. All our usages of these methods use a block to add content so I've set it up to raise an error. I also removed the need to remove excess new lines after govspeak is rendered. Since we already have HTML at that point I'm not sure what value that adds.
This change allows template files to lack the .govspeak part of their filename. For example lib/smart_answer_flows/business-coronavirus-support-finder/business_coronavirus_support_finder.govspeak.erb can be renamed lib/smart_answer_flows/business-coronavirus-support-finder/business_coronavirus_support_finder.erb This is done to reflect that these files no longer only contain govspeak (arguably they never did since they were a mix of govspeak and plain text) and thus the govspeak extension adds confusion. Since they are mixed media it makes more sense for them to just have a ".erb" extension. Rendering and looking up of templates are changed to be done by the template lookup system of action view rather than the previous manual path manipulation. The lookup work only seems to be used as a means to output extra help text in views, which seems potentially unnecessary. I didn't want to remove this functionality here though as I don't know if someone does use it. I did move the erb_template_path to be private since nothing external seems to use it.
These tests were all tested functionality defined outside this test.
This adds in a check when specifying HTML in a smart answer (via govspeak_for or html_for) that it is not for a field that is text only. This is put in to address a concern that it could be rather easy for someone to specify `govspeak_for(:title) { "my govspeak" }` and this produce an unexpected output that no-one notices. As part of this I removed the govspeak option in DEFAULT_FORMATS as I didn't think it affected the outcome of any method calls.
This updates the documentation to remove references to .govspeak.erb files and to make suggestions of which helper method (text_for, govspeak_for, html_for) in each scenario. Along the way I noticed that the label field may not be working (issue prior to these changes) so I left a note about that so the next person who wants it can investigate. I removed the information on error_* as I believe that is mostly set on a custom basis so doesn't need to be included in the general documentation.
This uses the new text_for & govspeak_for helpers and drops the .govspeak extension from the filename.
0215636
to
28fb339
Compare
Since #4527 was merged we no longer use the render_content_for method in smart answers and can now safely remove it.
Since #4527 was merged we no longer use the render_content_for method in smart answers and can now safely remove it.
Addresses #4510 - I've opened this up as a draft, as a proposal to resolve the issue
This adds
text_for
,govspeak_for
andhtml_for
methods intendedfor the smart answer flow methods as a way for users to define content
for a smart answer. These are intended to replace the
render_content_for
helper that has logic to determine the contenttype.
This is set-up to be backwards compatible with the render_content_for
method so that both can co-exist for a period of time to allow
migration.
In doing this I also simplified a few things. We don't make any use of
options when calling
content_for
so these methods don't need tosupport an option. All our usages of these methods use a block to add
content so I've set it up to raise an error.